TCXB8-4164: Re-enable profile scheduler when hash-match skips dead thread - #410
TCXB8-4164: Re-enable profile scheduler when hash-match skips dead thread#410tabbas651 wants to merge 2 commits into
Conversation
…read When a profile's TimeoutThread dies silently (e.g. pthread init failure at boot during GFO restore), subsequent WebConfig or XConf pushes with the same hash skip re-registration, leaving the profile permanently unscheduled. Add isProfileSchedulerRunning() to check if a profile's scheduler thread is alive. In both msgpack and JSON hash-match paths, call enableProfile() to restart the scheduler when the thread is not running.
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent report profiles from becoming permanently unscheduled when their scheduler (TimeoutThread) dies silently and subsequent config pushes are skipped due to hash-match optimization. It introduces a scheduler-liveness check and attempts to re-enable profiles when the config hash hasn’t changed but scheduling is no longer active.
Changes:
- Added
isProfileSchedulerRunning(const char*)to the scheduler public API. - Implemented the liveness check in
scheduler.c. - Updated both JSON and msgpack hash-match paths in
reportprofiles.cto re-enable the profile if the scheduler is not running.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| source/scheduler/scheduler.h | Exposes new scheduler liveness API. |
| source/scheduler/scheduler.c | Implements isProfileSchedulerRunning() to detect scheduler status per profile. |
| source/bulkdata/reportprofiles.c | On hash-match, attempts to re-enable profiles when scheduler is not running (JSON + msgpack paths). |
Suppressed comments (2)
source/bulkdata/reportprofiles.c:1000
- Calling enableProfile(profileName) here is unlikely to restart a dead scheduler thread: enableProfile() returns early when profile->enable is already true, and registerProfileWithScheduler() also exits when a scheduler entry already exists for the profile name. If the existing scheduler entry’s TimeoutThread died silently, this path will still leave the profile unscheduled. A dedicated scheduler restart path (or making registerProfileWithScheduler recreate the thread when the existing one is not alive) seems necessary to meet the PR description.
if(!isProfileSchedulerRunning(profileName))
{
T2Warning("Profile %s scheduler not running, re-enabling\n", profileName);
enableProfile(profileName);
}
source/bulkdata/reportprofiles.c:1415
- Same as the JSON path above: enableProfile(profileName) will no-op when the profile is already enabled, and registerProfileWithScheduler() returns success when an entry already exists, so this is unlikely to restart a dead TimeoutThread. Consider adding a scheduler "restart" API or updating registerProfileWithScheduler() to recreate the thread when the existing one is not alive.
if(!isProfileSchedulerRunning(profileName))
{
T2Warning("Profile %s scheduler not running, re-enabling\n", profileName);
enableProfile(profileName);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
source/bulkdata/reportprofiles.c:1415
- Same issue as the JSON path: enableProfile(profileName) will not restart scheduling if the profile is already enabled (enableProfile() returns early when profile->enable is true; see source/bulkdata/profile.c:1039-1044). This means the intended recovery for ‘hash match but dead scheduler thread’ will not occur.
if(!isProfileSchedulerRunning(profileName))
{
T2Warning("Profile %s scheduler not running, re-enabling\n", profileName);
enableProfile(profileName);
}
source/bulkdata/reportprofiles.c:1000
- The new ‘hash matches but scheduler thread is not running’ recovery path is not covered by unit tests. There is existing bulkdata unit test coverage for ReportProfiles_ProcessReportProfilesBlob / msgpack processing (source/test/bulkdata/reportprofilesTest.cpp), but no test exercises the hash-match branch with isProfileSchedulerRunning() returning false and verifies the expected recovery action is taken.
if(!isProfileSchedulerRunning(profileName))
{
T2Warning("Profile %s scheduler not running, re-enabling\n", profileName);
enableProfile(profileName);
}
source/bulkdata/reportprofiles.c:1000
- Calling enableProfile(profileName) here will not restart a dead scheduler thread when the Profile is already marked enabled. enableProfile() returns early for enabled profiles (source/bulkdata/profile.c:1039-1044), so this new recovery path is effectively a no-op in the failure scenario described in the PR (thread init failure but profile stays enabled). Consider adding an explicit ‘restart scheduler’ path (e.g., force re-register with scheduler when enabled but scheduler not running) instead of calling enableProfile unconditionally.
This issue also appears in the following locations of the same file:
- line 996
- line 1411
if(!isProfileSchedulerRunning(profileName))
{
T2Warning("Profile %s scheduler not running, re-enabling\n", profileName);
enableProfile(profileName);
}
source/scheduler/scheduler.c:453
- isProfileSchedulerRunning() uses pthread_kill(tProfile->tId, 0) to test liveness, but scheduler.c currently ignores pthread_create() failures when setting tProfile->tId (registerProfileWithScheduler() calls pthread_create without checking its return). In the PR’s described failure mode (thread creation/init failure), tProfile->tId may be left uninitialized/invalid, and calling pthread_kill on an invalid pthread_t is undefined on some pthread implementations. Consider recording thread-creation success (or only storing profiles in profileList after a successful pthread_create) so this liveness check is safe and reliable.
bool isProfileSchedulerRunning(const char* profileName)
{
if(!sc_initialized || profileName == NULL || profileList == NULL)
return false;
if(pthread_mutex_lock(&scMutex) != 0)
return false;
size_t index = 0;
for(; index < profileList->count; ++index)
{
SchedulerProfile *tProfile = (SchedulerProfile *)Vector_At(profileList, index);
if(tProfile == NULL || tProfile->name == NULL)
continue;
if(strcmp(tProfile->name, profileName) == 0)
{
/* pthread_kill with signal 0 checks if the thread is still alive */
bool running = (pthread_kill(tProfile->tId, 0) == 0);
pthread_mutex_unlock(&scMutex);
return running;
When a profile's TimeoutThread dies silently (e.g. pthread init failure at boot during GFO restore), subsequent WebConfig or XConf pushes with the same hash skip re-registration, leaving the profile permanently unscheduled.
Add isProfileSchedulerRunning() to check if a profile's scheduler thread is alive. In both msgpack and JSON hash-match paths, call enableProfile() to restart the scheduler when the thread is not running.